fix(orchestrator): stop the probe PR resolver re-walking the whole PR tree - #377

A sweep went silent for 11m53s and then reported stalled (inFlightMs 727833, missedPasses 12, ratio 0.985) while the process stayed healthy — liveHeartbeat logging throughout, log ring lossless (droppedBytes 0), consecutiveFailures 0. Nothing was failing. The sweep was slow by construction, inside resolveIssuePrFromMount, which answers "which PR belongs to this issue" by reading every mounted PR record one at a time.

10:25:30.117Z listTree /cloud/pulls/by-id/ -> 1156 paths   <-- LAST LINE THE SWEEP EMITS
10:25:30 -> 10:37:23Z  silence
10:37:23Z     stalled

Relationship to #374

Complementary, not redundant.

Rebased onto 00d51ad; no conflicts. sweep-budget.test.ts (all 810 lines of #374's suite) passes alongside this change. No instrumentation collision: #374's budget?.assertNotExpired() and the ready-issue read-progress idiom already coexist in the same loop (factory.ts:3652-3658); A4 mirrors the logging half of that loop, so the two mechanisms sit side by side exactly as they already do upstream.

The four defects

1. #resolveIssuePr could not scope to a repository

resolveIssuePrFromMount has always honoured opts.repo, but #resolveIssuePr's own opts had no repo field, so it forwarded undefined and walked EVERY configured repository — 21 in the live workspace — to find a PR that can only live in one. None of its four call sites nor the probePrResolver port could scope it.

Adds repo?: string and threads the routing answer through all of them via a new #probeRepoForIssue, which reuses dependencyRepoForIssue — the same helper #dependencyIsTerminalOrMerged already uses for its own probe. Ambiguous routing (multi-route decision, unlabelled issue) still walks unscoped: narrowing on a guess would silently miss a PR that is really there.

Changed: #probeRepoForIssue (new, factory.ts:3021), opts field at 3033, call sites at 2991, 3002, 4750, 19033, port at 1204.

2. The mount hit never populated the cache it reads

#resolveIssuePr reads #probePrResolvedCache at the top, then runs the mount walk first — the common hit — and returned without ever writing it. The cache had a reader and no writer on the hot path, so the whole walk repeated per caller, per sweep, forever.

Now cached on the same terms as the gh branch below it: same key, same TTL, same draft exclusion (the reason to keep a draft uncached is a property of the PR, not of which resolver observed it).

Changed: factory.ts:3053-3065.

3. The walk read most pull requests twice

githubPullRoots returns two roots for one repository — the nested <owner>/<repo>/pulls/ layout and the flat <owner>__<repo>/pulls/by-id/ alias — and unions them into a Set keyed by path string, so one PR under two spellings counted twice. That is the 2877 + 1156 = 4033 in the repro for roughly 1156 actual pull requests.

Deduped on the identity the path already carries, via githubPullPathParts — no mount read, so the dedupe is free. Insertion order is preserved and the first spelling wins, which is the candidate the existing stable sort already kept when two spellings of one PR tied, so the winner does not move. Paths carrying no PR identity (_index.json, per-PR comments/*.json) stay in the walk and are read exactly as before rather than filtered on a guess.

Changed: factory.ts:20643-20674.

4. The read loop was invisible

listTree is wrapped by #listRelayfileTree — named, timed, logged. The readFile per candidate ran inside a bare try/catch that swallows failures into undefined, with no logger, no counter and no progress line. That is why twelve minutes of real work was indistinguishable from a hung process for three prior investigation layers; the observability gap is a first-class defect here, not a nice-to-have.

Adds progress reporting on #logTimedProgress — the same cadence helper the ready-issue read loop uses — plus a probePrMountReads counter.

Changed: #probeMountWalkProgress (new, factory.ts:5206), wired at 3050 and 9280.

Two more found while fixing the above

The cache invalidation was already broken. On completion it deleted only the bare issue key, never the :open / :legacy suffixed variants #resolveIssuePr actually writes — so every openOnly probe (#openPrForIssue, #openCompletionPr, i.e. the completion path) was never invalidated at all. Harmless while the mount branch wrote nothing; a live correctness bug the moment it does. Now clears the whole key family (factory.ts:16024-16041).

#dependencyIsTerminalOrMerged had no cache at all — and it is the path that produced the repro. It calls resolveIssuePrFromMount directly (it must not fall back to gh), so it never saw #resolveIssuePr's cache, and #terminalDependencyIdentities memoises only the TRUE answer. A dependency that is not merged was re-walked in full for every issue declaring it, on every sweep. Adds a sweep-scoped memo for the negative answer, cleared beside the terminal set so a PR merging between sweeps is still observed (factory.ts:812, 3605, 9273).

This corrects the framing in the original diagnosis. The verified repro runs through #dependencyIsTerminalOrMerged, which already passed repo and never touched #probePrResolvedCache. So defects 1 and 2 — real as they are — do not fix the observed 11m53s stall; they fix the aggravated 21-repo variant. What fixes the repro is defect 3, the dependency memo, and defect 4 making it visible.

What is NOT fixed, deliberately

No early break on a maximal-score match. The sort is b.score - a.score || b.prNumber - a.prNumber, so a score-30 hit does not win until every higher-numbered candidate is known to score no better; and readProbePrCandidate takes pr.number from the payload rather than the path, so path order does not prove PR-number order. Semantics could not be shown preserved, so per the brief the dedupe ships and the early break does not.

No index fast path. pulls/_index.json rows carry no headRef, and the primary match (score 30) is a branch match — so the index cannot exclude any PR from consideration, and a title hit (score 20) must never be returned as the answer while an unread branch match could outrank it. Note this is a stronger objection than "the index is the wrong shape": it holds even on a well-formed index. Instead the resolver now logs why it fell back (index-absent / index-shape-unrecognised / index-without-head-ref / index-usable), so the day adapters#271 lands shows up in the logs rather than passing unnoticed.

No bounded concurrency in the read loop. It would fix wall-clock without changing read count, but it is a behavioural change to the hot path that nobody asked for; recommended as a follow-up.

On _index.json shapes — evidence

Verified directly against the relayfile-adapters checkout at e6edb075:

• index-emitter.ts:112-134 (buildRepoIssuesIndexFile / buildRepoPullsIndexFile) writes a bare top-level array at the canonical nested path — the shape Factory's reader accepts. Confirmed by bulk-ingest.test.ts:490, which asserts pulls/_index.json parses to [{ id, title, updated, number, state, merged, mergedAt }].
• lazy.ts:161,179 (eager backfill) writes { issues: [...] } / { pulls: [...] } — object-wrapped — to the same canonical path.
• bulk-writer.ts:902 writes a directory manifest at the flat alias path; it contains no records.

Answering the question on #githubIssuePathsFromIndex (factory.ts:8759): it is conditionally, not universally, falling back. On mounts last written by the incremental index emitter the shape is the bare array it accepts and labels is present on issue rows (added for exactly this gate, index-emitter.ts:20-23), so it works. On eager-backfilled mounts it gets the { issues: [...] } object, fails Array.isArray, and silently falls back to the tree walk. Same canonical path, two writers, so which behaviour you get depends on which writer touched it last. Not fixed here — separate lane.

Red-then-green evidence

Every test pinned on read count, not wall clock, using FakeMountClient.reads. All measured on the rebased tree (378bd95 on 00d51ad), each fix reverted independently:

The first is the O(N) vs O(N×R) assertion: 4 PRs in each of 3 configured repos, issue routed to one — 5 reads scoped, 13 unscoped.

Test results

$ ./node_modules/.bin/vitest run src/orchestrator/factory.test.ts
 Test Files  1 passed (1)
      Tests  621 passed (621)
   Duration  311.76s

$ ./node_modules/.bin/vitest run src/orchestrator/sweep-budget.test.ts \
    src/orchestrator/sweep-counters.test.ts src/orchestrator/dispatch-failure-reasons.test.ts \
    src/orchestrator/health-projection-guard.test.ts src/writeback/writeback.test.ts \
    src/node/factory-node.test.ts
 Test Files  6 passed (6)
      Tests  143 passed (143)

Environment caveat, stated plainly: the sandbox has no access to the private npm registry, so npm ci could not install this branch's dependency tree. Tests ran against an overlay of the nearest locally-available packages. Two consequences, both verified rather than assumed:

• src/node/factory-persona-card.test.ts fails 7/7 — it needs @relaycast/a2a@^6.2.0 and only 1.1.7 was available locally. Confirmed pre-existing: identical 7 failures with my changes stashed on pristine main.
• tsc --noEmit reports 225 errors repo-wide from the same version skew (e.g. @relayfile/sdk missing exports, Promise.withResolvers needing a newer lib). Zero of them are in src/orchestrator/factory.ts.

Scope

Touches only src/orchestrator/factory.ts and src/orchestrator/factory.test.ts, per the gate.

🤖 Generated with Claude Code